Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockchain: update BestState. #1416

Merged
merged 1 commit into from
Aug 23, 2018
Merged

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Aug 22, 2018

This adds PrevHash, NextPoolSize, NextStakeDiff, NextWinningTickets, MissedTickets and NextFinalState fields to BestState.

Port of upstream commit c57c18c. In preparation of the removal of chainState.

TotalTxns uint64 // The total number of txns in the chain.
MedianTime time.Time // Median time as per CalcPastMedianTime.
TotalSubsidy int64 // The total subsidy for the chain.
WinningTickets []chainhash.Hash // The eligigle tickets to vote on the next block.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eligible

TotalSubsidy int64 // The total subsidy for the chain.
WinningTickets []chainhash.Hash // The eligigle tickets to vote on the next block.
MissedTickets []chainhash.Hash // The missed tickets set to be revoked.
FinalState [6]byte // The calculated state of the lottery.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is never set. Please don't add any fields that are invalid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow up, this can simply be set in newBestState from node.finalState.

@@ -934,9 +951,15 @@ func (b *BlockChain) disconnectBlock(node *blockNode, block, parent *dcrutil.Blo
subsidy := CalculateAddedSubsidy(block, parent)
newTotalSubsidy := curTotalSubsidy - subsidy

// Calcultate the next stake difficulty.
nextStakeDiff, err := b.CalcNextRequiredStakeDifficulty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. It would calculate the stake diff for the block after the one being disconnected. You can just use the data that is already in node.sbits though since it's about to be disconnected and hence is the next stake difficulty as compared to the previous block for which the new state represents.

@@ -1463,12 +1463,19 @@ func (b *BlockChain) createChainState() error {
// genesis block, use its timestamp for the median time.
numTxns := uint64(len(genesisBlock.MsgBlock().Transactions))
blockSize := uint64(genesisBlock.MsgBlock().SerializeSize())

// Calcultate the next stake difficulty.
nextStakeDiff, err := b.CalcNextRequiredStakeDifficulty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to calculate this when creating a chain state for the genesis block. The next stake difficulty is already well known for the block after the genesis block. In particular, it's b.chainParams.MinimumStakeDiff.

@@ -761,9 +772,15 @@ func (b *BlockChain) connectBlock(node *blockNode, block, parent *dcrutil.Block,
// Calculate the exact subsidy produced by adding the block.
subsidy := CalculateAddedSubsidy(block, parent)

// Calcultate the next stake difficulty.
nextStakeDiff, err := b.CalcNextRequiredStakeDifficulty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use b.calcNextRequiredStakeDifficulty(node) instead to be sure you're calculating it for the correct node. Otherwise, the tip isn't updated yet, so you'll be off by one.

@@ -1770,9 +1777,16 @@ func (b *BlockChain) initChainState(interrupt <-chan struct{}) error {
block := utilBlock.MsgBlock()
blockSize := uint64(block.SerializeSize())
numTxns := uint64(len(block.Transactions))

// Calcultate the next stake difficulty.
nextStakeDiff, err := b.CalcNextRequiredStakeDifficulty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use b.calcNextRequiredStakeDifficulty(tip) instead to be sure you're calculating it for the correct node.

MedianTime: medianTime,
TotalSubsidy: totalSubsidy,
Hash: node.hash,
PrevHash: node.parent.hash,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will crash if called on a new chain with the genesis block, because it has not parent.

Needs to be:

   prevHash := zeroHash
    if node.parent != nil {
        prevHash = &node.parent.hash
    }
    return &BestState{
        ...
        PrevHash: *prevHash,
        ...
    }

TotalTxns: totalTxns,
MedianTime: medianTime,
TotalSubsidy: totalSubsidy,
WinningTickets: node.stakeNode.Winners(),
Copy link
Member

@davecgh davecgh Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could easily crash with node.stakeNode not being loaded since the field is dynamically loaded. You need to make sure that you call fetchStakeNode prior to invoking newBestState with these changes.

In the case of connectBlock and disconnectBlock, there are already calls to load the stake nodes, but they are after the calls to newBestState, so the loading code should be moved above the blocks of code which deal with setting the best state.

For example:

diff --git a/blockchain/chain.go b/blockchain/chain.go
index 4856d241..3bc0c86a 100644
--- a/blockchain/chain.go
+++ b/blockchain/chain.go
@@ -747,6 +747,15 @@ func (b *BlockChain) connectBlock(node *blockNode, block, parent *dcrutil.Block,
                return err
        }

+       // Get the stake node for this node, filling in any data that
+       // may have yet to have been filled in.  In all cases this
+       // should simply give a pointer to data already prepared, but
+       // run this anyway to be safe.
+       stakeNode, err := b.fetchStakeNode(node)
+       if err != nil {
+               return err
+       }
+
        // Generate a new best state snapshot that will be used to update the
        // database and later memory if all database updates are successful.
        b.stateLock.RLock()
@@ -765,15 +774,6 @@ func (b *BlockChain) connectBlock(node *blockNode, block, parent *dcrutil.Block,
        state := newBestState(node, blockSize, numTxns, curTotalTxns+numTxns,
                node.CalcPastMedianTime(), curTotalSubsidy+subsidy)

-       // Get the stake node for this node, filling in any data that
-       // may have yet to have been filled in.  In all cases this
-       // should simply give a pointer to data already prepared, but
-       // run this anyway to be safe.
-       stakeNode, err := b.fetchStakeNode(node)
-       if err != nil {
-               return err
-       }
-
        // Atomically insert info into the database.
        err = b.db.Update(func(dbTx database.Tx) error {
                // Update best block state.

In the case of the init path, you'll need to load the stake nodes.

@dnldd dnldd force-pushed the port_update_best_state branch 2 times, most recently from 992e591 to 2b5483c Compare August 22, 2018 06:11
blockSize := uint64(block.MsgBlock().Header.Size)
state := newBestState(node, blockSize, numTxns, curTotalTxns+numTxns,
node.CalcPastMedianTime(), curTotalSubsidy+subsidy)
// Calcultate the next stake difficulty.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calculate

prevHash = node.parent.hash
}

winners := make([]chainhash.Hash, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. Please see my previous comments about the need to call fetchStakeNode and from where.

@dnldd dnldd force-pushed the port_update_best_state branch 4 times, most recently from ee65f1b to d79bf00 Compare August 22, 2018 07:52
@davecgh
Copy link
Member

davecgh commented Aug 22, 2018

Commit message and PR desc needs to be updated to reflect reality.

@dnldd dnldd force-pushed the port_update_best_state branch 3 times, most recently from d08358d to e608cf0 Compare August 22, 2018 22:00
This adds PrevHash, NextPoolSize, NextStakeDiff, NextWinningTickets,
MissedTickets  and NextFinalState fields to BestState.

Port of upstream commit c57c18c. In preparation of the removal of
chainState.
@dnldd dnldd force-pushed the port_update_best_state branch from e608cf0 to c540c2c Compare August 22, 2018 22:03
@davecgh davecgh added this to the 1.4.0 milestone Aug 22, 2018
@davecgh davecgh merged commit c540c2c into decred:master Aug 23, 2018
@davecgh davecgh mentioned this pull request Aug 28, 2018
33 tasks
JFixby pushed a commit to JFixby/dcrd that referenced this pull request Sep 20, 2019
…c-peer

netsync/manager: prioritize higher height peers for sync peer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants